feat: unified message banner for all screens#604
Conversation
Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request implements a comprehensive migration from scattered per-screen error messaging and status state to a centralized global Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
src/ui/dashpay/qr_scanner.rs (1)
323-369:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_scanner.rs` around lines 323 - 369, The QRScannerScreen impl of ScreenLike is missing the lifecycle methods required by the trait; add no-op implementations (or appropriate behavior) for refresh() or refresh_on_arrival() (whichever the trait defines) and change_context(&mut self, ctx: AppContext) to the impl block for QRScannerScreen so the trait is fully implemented; keep them minimal (empty bodies or a small update) matching the trait signatures used across src/ui/**/*.rs and reference the ScreenLike impl for QRScannerScreen so the compiler recognizes methods like refresh()/refresh_on_arrival() and change_context().src/ui/tokens/view_token_claims_screen.rs (1)
70-97:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/view_token_claims_screen.rs` around lines 70 - 97, The ScreenLike impl for ViewTokenClaimsScreen is missing required lifecycle methods; add the missing methods refresh (or refresh_on_arrival, depending on the trait variant) and change_context to the impl so the struct fully implements ScreenLike. Locate the impl ScreenLike for ViewTokenClaimsScreen and add fn refresh(&mut self) { /* no-op */ } or fn refresh_on_arrival(&mut self) { /* no-op */ } as appropriate to your ScreenLike trait, plus fn change_context(&mut self, ctx: AppContext) { /* update/ignore ctx as needed */ } using the exact method names and signatures from the ScreenLike trait so the file compiles and behavior remains unchanged.src/ui/contracts_documents/register_contract_screen.rs (2)
341-377:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/register_contract_screen.rs` around lines 341 - 377, Add the missing ScreenLike lifecycle methods to the impl for RegisterDataContractScreen: implement refresh(&mut self, ctx: &egui::Context) and refresh_on_arrival(&mut self, ctx: &egui::Context) (they can be no-ops) and implement change_context(&mut self, ctx: ScreenContext) to satisfy the ScreenLike trait; place them alongside the existing display_message and display_task_result methods and follow the trait signatures used across other screens so the compiler recognizes the impl of ScreenLike for RegisterDataContractScreen.
67-73:⚠️ Potential issue | 🟡 MinorSurface wallet-selection errors during initialization.
get_selected_walletcan populateerror_message, but it’s currently ignored innew(). After removing per-screen error state, this makes initial wallet failures silent. Consider surfacing via the global banner.Proposed fix
let selected_wallet = if let Some(ref identity) = selected_qualified_identity { get_selected_wallet(identity, Some(app_context), None, &mut error_message) } else { None }; + if let Some(err) = error_message { + MessageBanner::set_global(app_context.egui_ctx(), &err, MessageType::Error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/register_contract_screen.rs` around lines 67 - 73, The initialization in new() calls get_selected_wallet(...) which can set error_message but currently ignores it; update new() to check error_message after the call to get_selected_wallet and, if Some(msg), surface it to the global/banner UI via the app_context (e.g., call the app_context or global banner enqueue/display method) so initial wallet-selection failures are visible; ensure you still return the selected_wallet as before but also forward the error string to the global banner handling function.src/ui/tools/grovestark_screen.rs (1)
984-1003:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen already implements
refreshandrefresh_on_arrival, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/grovestark_screen.rs` around lines 984 - 1003, Add the missing change_context method to GroveSTARKScreen to satisfy the ScreenLike trait: implement fn change_context(&mut self, new_context: AppContext) (or the exact signature used in ScreenLike) to update the screen's stored app_context (and any dependent state) and trigger necessary updates (e.g., call self.refresh() or self.refresh_on_arrival() as appropriate); locate GroveSTARKScreen and implement change_context to replace/clone the existing app_context field and then call the existing refresh/refresh_identities(&app_context)/refresh_contracts(&app_context) flow so the screen reflects the new context.src/ui/tools/platform_info_screen.rs (1)
141-225:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen already implements
refreshandrefresh_on_arrival, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/platform_info_screen.rs` around lines 141 - 225, Add the missing change_context implementation in the impl ScreenLike for PlatformInfoScreen: implement fn change_context(&mut self, app_context: Arc<AppContext>) so it replaces the screen's AppContext (self.app_context = app_context), updates dependent cached fields (e.g. self.network = self.app_context.network), and clears or resets transient UI state such as self.platform_version, self.core_chain_lock_height, self.current_result, self.current_result_title and self.active_tasks; ensure the method signature and ownership match the ScreenLike trait definition used elsewhere.src/ui/tokens/direct_token_purchase_screen.rs (1)
346-387:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen implements
refresh, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/direct_token_purchase_screen.rs` around lines 346 - 387, Implement the missing ScreenLike::change_context(&mut self, ctx: &AppContext) for PurchaseTokenScreen: add a method named change_context that updates the screen's stored app context reference/handle (the same field used elsewhere, e.g., self.app_context), clears or resets transient UI state such as self.refresh_banner and any error status, and then triggers the same refresh behavior used on arrival (call the existing refresh() or refresh_on_arrival() path used by other screens). Follow the pattern from other screens implementing ScreenLike so the screen's fields (fetched_pricing_schedule, pricing_fetch_attempted, status, refresh_banner) are correctly reinitialized when context changes.src/ui/mod.rs (1)
837-846: 🛠️ Refactor suggestion | 🟠 MajorAdd
change_context()method to ScreenLike trait.The trait is missing
change_context()required by the coding guidelines. Add it as a default no-op and delegate in theScreenimpl to match the pattern used for other optional methods.Suggested changes
pub trait ScreenLike { fn refresh(&mut self) {} fn refresh_on_arrival(&mut self) { self.refresh() } fn ui(&mut self, ctx: &Context) -> AppAction; fn display_message(&mut self, _message: &str, _message_type: MessageType) {} fn display_task_result(&mut self, _backend_task_success_result: BackendTaskSuccessResult) {} + fn change_context(&mut self, _app_context: Arc<AppContext>) {} fn pop_on_success(&mut self) {} }impl ScreenLike for Screen { + fn change_context(&mut self, app_context: Arc<AppContext>) { + Screen::change_context(self, app_context); + } fn refresh(&mut self) { match self { Screen::IdentitiesScreen(screen) => screen.refresh(), // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/mod.rs` around lines 837 - 846, The ScreenLike trait needs a new optional method change_context() following the same pattern as the other no-op methods: add fn change_context(&mut self) {} to the ScreenLike trait as a default no-op, and in the Screen impl add a corresponding pub fn change_context(&mut self) { self.screen.change_context(); } (or delegate from whatever wrapper method name you use) so the Screen wrapper simply forwards change_context to the inner ScreenLike implementation.src/ui/wallets/send_screen.rs (1)
2580-2723:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 2580 - 2723, The ScreenLike implementation for WalletSendScreen is missing the required change_context() method; add a public impl method fn change_context(&mut self, ctx: &AppContext) { ... } (or matching your trait signature) to the impl ScreenLike for WalletSendScreen so the type fulfills the ScreenLike contract. Locate the impl block containing ui(), display_message(), display_task_result(), refresh_on_arrival(), and refresh(), and add change_context() alongside those methods, updating any internal state (e.g., self.app_context or selected_wallet) as your trait expects.src/ui/tools/transition_visualizer_screen.rs (1)
386-431:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/transition_visualizer_screen.rs` around lines 386 - 431, The impl ScreenLike for TransitionVisualizerScreen is missing the required change_context method; add a method named change_context to this impl with the exact signature defined by the ScreenLike trait (e.g., fn change_context(&mut self, ctx: &mut AppContext) or the crate::ui equivalent) and implement it to update the screen's internal state from the provided context (similar to how ui()/refresh()/display_message()/display_task_result() operate), placing it inside the impl block for TransitionVisualizerScreen so the type fully satisfies the ScreenLike contract.src/ui/dashpay/send_payment.rs (1)
379-449:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/send_payment.rs` around lines 379 - 449, The impl for ScreenLike on SendPaymentScreen is missing the required change_context method; add a change_context implementation matching the ScreenLike trait signature (same params/borrow patterns as other screens) inside the impl for SendPaymentScreen, update the internal app context/state (self.app_context or related fields) from the new context, and call any necessary refresh logic like self.load_contact_info() or self.refresh() so the screen updates when the app context changes; reference: implement change_context for SendPaymentScreen within the existing impl ScreenLike block so it mirrors other screens' behavior.src/ui/tokens/resume_tokens_screen.rs (1)
278-307:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl doesn’t provide
change_context(). Please add it to align with the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/resume_tokens_screen.rs` around lines 278 - 307, Add the missing ScreenLike::change_context implementation for ResumeTokensScreen: implement fn change_context(&mut self, ctx: ScreenContext) (matching the ScreenLike trait) that updates the screen's stored context (assign ctx to the screen's context field, e.g., self.context) and then trigger the appropriate refresh behavior (call refresh_on_arrival() if present else refresh()) so the screen state is updated when context changes; ensure the method lives in the same impl block for ResumeTokensScreen alongside display_message/display_task_result/refresh.src/ui/contracts_documents/update_contract_screen.rs (2)
362-398:⚠️ Potential issue | 🟡 MinorAdd change_context to ScreenLike impl.
This ScreenLike impl still lacks
change_context(). Please add it to meet the screen contract.As per coding guidelines,
src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/update_contract_screen.rs` around lines 362 - 398, The impl for ScreenLike on UpdateDataContractScreen is missing the required change_context method; add a change_context implementation matching the ScreenLike trait signature (change_context(...)) inside the impl block for UpdateDataContractScreen, and have it update the screen's context or call the existing refresh/refresh_on_arrival logic (e.g., update any context-related field on UpdateDataContractScreen and trigger refresh behavior) so the type satisfies the ScreenLike contract.
35-44:⚠️ Potential issue | 🟠 MajorBroadcastError blocks retry because the parsed contract is discarded.
After an error,
BroadcastStatus::BroadcastErroris set and theValidContractpayload is lost, so the Update button never reappears unless the user edits the JSON to re-parse. Consider rehydrating the parsed contract (or resetting to a retryable state) when a broadcast error occurs.✅ Suggested fix (rehydrate for retry)
- } else { - self.broadcast_status = BroadcastStatus::BroadcastError; - } + } else { + // Re-parse to restore ValidContract so the user can retry without editing JSON. + self.parse_contract(); + }Also applies to: 186-222, 364-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/update_contract_screen.rs` around lines 35 - 44, The BroadcastError path currently drops the parsed contract (BroadcastStatus::BroadcastError) so retries are blocked; preserve the parsed contract by changing the enum (in src/ui/contracts_documents/update_contract_screen.rs) to carry the contract (e.g., BroadcastError(Box<DataContract>, Option<String>) or add a new variant RetryableBroadcastError(Box<DataContract>, Option<String>)), update all places that set BroadcastStatus::BroadcastError (and any match arms handling it) to supply the parsed contract and error message, and update the UI logic that decides whether to show the Update button to treat the new/updated BroadcastError variant as retryable (equivalent to ValidContract) so the user can retry without re-parsing.src/ui/identities/keys/add_key_screen.rs (1)
410-413:⚠️ Potential issue | 🟡 MinorDuplicate unreachable
Completecheck.Lines 402-405 already check
AddKeyStatus::Completeand return early. This second check at lines 410-413 is dead code.🧹 Remove duplicate check
ui.heading("Add New Key"); ui.add_space(10.0); - if self.add_key_status == AddKeyStatus::Complete { - inner_action |= self.show_success(ui); - return inner_action; - } - if self.selected_wallet.is_some()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/add_key_screen.rs` around lines 410 - 413, Remove the duplicate unreachable check for AddKeyStatus::Complete: the code block that tests `if self.add_key_status == AddKeyStatus::Complete { inner_action |= self.show_success(ui); return inner_action; }` is redundant because an earlier return already handles `AddKeyStatus::Complete`; delete this second check (the duplicate `Complete` branch) so `self.add_key_status` is only handled once and flow continues to the intended subsequent logic (leave `show_success` and `inner_action` usage only in the original place where it's returned).
🟡 Minor comments (11)
src/ui/identities/transfer_screen.rs-502-510 (1)
502-510:⚠️ Potential issue | 🟡 MinorAdd missing
change_contextimplementation.This screen implements
refresh, butchange_contextis still missing. Please add it per the ScreenLike trait contract.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/transfer_screen.rs` around lines 502 - 510, The TransferScreen impl for ScreenLike is missing the required change_context(&mut self, ctx: ScreenContext) method; add a change_context implementation inside impl ScreenLike for TransferScreen that matches the ScreenLike trait signature (change_context), update TransferScreen's internal context/state as appropriate (e.g., store the incoming ScreenContext or update fields used by ui()/refresh()/refresh_on_arrival()), and trigger any necessary side-effects such as scheduling a refresh or clearing/setting refresh_banner similar to display_message/refresh behavior so the screen reacts to context changes consistently.src/ui/tokens/update_token_config.rs-918-934 (1)
918-934:⚠️ Potential issue | 🟡 MinorImplement missing ScreenLike lifecycle methods.
This impl lacks
refresh/refresh_on_arrivalandchange_context, which are required for screens insrc/ui/**/*.rs. Please add the methods (even as no-ops) per the trait signature.
As per coding guidelines:src/ui/**/*.rs: All screens must implement theScreenLiketrait with methods:ui(),display_task_result(),display_message(),refresh()/refresh_on_arrival(), andchange_context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/update_token_config.rs` around lines 918 - 934, The impl for UpdateTokenConfigScreen is missing the ScreenLike lifecycle methods refresh/refresh_on_arrival and change_context; add no-op implementations that match the ScreenLike trait signatures (i.e., implement refresh() and/or refresh_on_arrival() and change_context() for UpdateTokenConfigScreen) alongside the existing display_message and display_task_result methods so the type fully implements ScreenLike. Ensure the method names and signatures exactly match the trait used by other screens (use those files as reference) and keep bodies empty or performing no-ops consistent with coding guidelines.src/ui/dashpay/qr_scanner.rs-50-58 (1)
50-58:⚠️ Potential issue | 🟡 MinorClear stale parsed QR data when input is empty.
If the user clears the input and clicks “Parse QR Code,”
parsed_qr_dataremains from a previous parse, so “Add Contact” can send outdated data. Reset it before returning.Proposed fix
if self.qr_data_input.is_empty() { + self.parsed_qr_data = None; crate::ui::components::MessageBanner::set_global( self.app_context.egui_ctx(), "Please enter QR code data", MessageType::Error, ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_scanner.rs` around lines 50 - 58, In parse_qr_code, when self.qr_data_input is empty you currently show an error and return but do not clear stale parsed_qr_data; update parse_qr_code to reset self.parsed_qr_data to None (or the empty/default value used by the struct) before returning so the previous parsed QR payload cannot be reused by the "Add Contact" flow; locate the parsed_qr_data field on the same struct and clear it at the top of parse_qr_code right before the MessageBanner and return.docs/ai-design/2026-02-17-unified-messages/message-banner-ux-spec.md-31-35 (1)
31-35:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
This resolves markdownlint warnings and improves formatting consistency.💡 Suggested fix
-``` +```text +-----------------------------------------------------------------------+ | [Icon] Message text here [5s] [x] | +-----------------------------------------------------------------------+ -``` +``` -``` +```text +--------------------------------------------------+ | Top Panel (header / navigation) | +--------------------------------------------------+ | Left Panel | [ Banner 1 ] | | | [ Banner 2 ] | | | +----- Screen Content -----+ | | | | ... | | | | +--------------------------+ | +--------------------------------------------------+ -``` +``` </details> Also applies to: 81-91 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/ai-design/2026-02-17-unified-messages/message-banner-ux-spec.mdaround
lines 31 - 35, Add language identifiers ("text") to the fenced code blocks for
the ASCII UI art examples so markdownlint MD040 is satisfied; update the three
backtick fences shown in the snippet (the banner box block and the larger layout
block—the same pattern also appears later around the second example) to use
text instead ofand ensure each opening fence has a matching closing
fence with no extra characters removed or added.</details> </blockquote></details> <details> <summary>docs/ai-design/2026-02-17-unified-messages/architecture.md-127-139 (1)</summary><blockquote> `127-139`: _⚠️ Potential issue_ | _🟡 Minor_ **Add language identifiers to fenced code blocks (MD040).** This clears markdownlint warnings and improves syntax highlighting. <details> <summary>💡 Suggested fix</summary> ```diff -``` +```rust render_banner(ui, text, message_type, annotation: Option<&str>) -> bool (dismissed?) ``` -``` +```text +-----------------------------------------------------------------------+ | [Icon] Message text here [5s] [x] | +-----------------------------------------------------------------------+ -``` +``` -``` +```text TaskResult::Error(message) → MessageBanner::set_global(ctx, &message, MessageType::Error) → screen.display_message(&message, MessageType::Error) // for side-effects ``` </details> Also applies to: 156-164 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/ai-design/2026-02-17-unified-messages/architecture.mdaround lines 127
- 139, Add explicit language identifiers to the fenced code blocks in the doc
snippets to satisfy MD040: update the first function block to userust for the render_banner signature (render_banner/ui/process_banner references), change the ASCII art and flow blocks totext (the visual structure box and the
TaskResult::Error → MessageBanner::set_global → screen.display_message
sequence), and ensure the other similar block at lines 156-164 also has a
language identifier; no code changes required, only annotate the three fenced
blocks with appropriate language tags.</details> </blockquote></details> <details> <summary>src/ui/tokens/tokens_screen/keyword_search.rs-63-70 (1)</summary><blockquote> `63-70`: _⚠️ Potential issue_ | _🟡 Minor_ **Elapsed timer can carry over across searches.** `MessageBanner::set_global` deduplicates by text, so repeating the same “Searching contracts...” banner can reuse the old `created_at` and the elapsed counter won’t reset on a new search. Consider clearing/replacing the prior banner handle before starting a new search. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/tokens_screen/keyword_search.rs` around lines 63 - 70, The elapsed timer can persist because MessageBanner::set_global deduplicates by text; before creating a new banner in keyword_search (where you call MessageBanner::set_global and assign to self.operation_banner), clear or remove any existing banner handle stored in self.operation_banner (e.g., if let Some(prev) = self.operation_banner.take() { prev.close() } or call the banner's removal API) so the old banner is removed and the new handle gets a fresh created_at; then call MessageBanner::set_global and store the new handle in self.operation_banner as you already do. ``` </details> </blockquote></details> <details> <summary>src/ui/tokens/pause_tokens_screen.rs-91-117 (1)</summary><blockquote> `91-117`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix “Burning” wording in pause authorization errors.** These messages are shown on the Pause screen and should say “pause,” not “burn.” <details> <summary>Suggested text fix</summary> ```diff - set_error_banner("Burning is not allowed on this token"); + set_error_banner("Pausing is not allowed on this token"); ... - "You are not allowed to burn this token. Only the contract owner is.", + "You are not allowed to pause this token. Only the contract owner is.", ... - set_error_banner("You are not allowed to burn this token"); + set_error_banner("You are not allowed to pause this token"); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/pause_tokens_screen.rs` around lines 91 - 117, Update the user-facing error strings in the pause authorization checks to refer to "pause" instead of "burn": modify the messages passed to set_error_banner in the AuthorizedActionTakers match arms (the "Burning is not allowed on this token", "You are not allowed to burn this token", and "You are not allowed to burn this token. Only the contract owner is.") so they read e.g. "Pausing is not allowed on this token", "You are not allowed to pause this token", and "You are not allowed to pause this token. Only the contract owner is." Reference set_error_banner, identity_token_info, and the AuthorizedActionTakers::NoOne/ContractOwner/Identity branches to locate and change the strings. ``` </details> </blockquote></details> <details> <summary>src/ui/tokens/resume_tokens_screen.rs-279-286 (1)</summary><blockquote> `279-286`: _⚠️ Potential issue_ | _🟡 Minor_ **Handle both Error and Warning messages to prevent stuck "resuming" state.** The `display_message` method currently only clears `refresh_banner` and sets error state for `Error` messages. However, `Warning` messages are also emitted in the UI (they don't auto-dismiss in the banner), and should be treated identically to prevent the "Resuming tokens..." banner from continuing indefinitely. This pattern is already established across multiple similar screen implementations (send_screen, add_contracts_screen, add_token_by_id_screen, etc.) which all handle both `Error` and `Warning` variants together. <details> <summary>✅ Suggested change</summary> ```diff - if let MessageType::Error = message_type { + if matches!(message_type, MessageType::Error | MessageType::Warning) { if let Some(h) = self.refresh_banner.take() { h.clear(); } self.status = ResumeTokensStatus::Error; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/resume_tokens_screen.rs` around lines 279 - 286, The display_message method only handles MessageType::Error; update it to treat MessageType::Warning the same way so warning banners also clear and the UI doesn't stay in "Resuming tokens..." state—locate the display_message function and change the conditional on MessageType (e.g., use a match or matches!() to check for MessageType::Error or MessageType::Warning), then keep the existing behavior: take and clear self.refresh_banner (if Some) and set self.status = ResumeTokensStatus::Error. ``` </details> </blockquote></details> <details> <summary>src/ui/contracts_documents/contracts_documents_screen.rs-213-218 (1)</summary><blockquote> `213-218`: _⚠️ Potential issue_ | _🟡 Minor_ **Clear any active query banner on parse errors.** If a user submits an invalid query while a previous “Querying documents…” banner is active, the stale banner remains visible. Clearing it here avoids misleading UI state. <details> <summary>🐛 Suggested fix</summary> ```diff Err(e) => { + if let Some(h) = self.query_banner.take() { + h.clear(); + } self.document_query_status = DocumentQueryStatus::Error; MessageBanner::set_global( ui.ctx(), &format!("Failed to parse query properly: {}", e), MessageType::Error, ); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/contracts_documents_screen.rs` around lines 213 - 218, On parse error update, clear any existing global banner before setting the error banner to avoid leaving the prior "Querying documents..." message visible: in the block where you set self.document_query_status = DocumentQueryStatus::Error and call MessageBanner::set_global(ui.ctx(), ...), first call the MessageBanner method that removes/clears the global banner (e.g., MessageBanner::clear_global(ui.ctx()) or the project's equivalent) and then set the new error banner via MessageBanner::set_global; keep the same context ui.ctx() and the formatted error text. ``` </details> </blockquote></details> <details> <summary>src/ui/dashpay/contacts_list.rs-910-913 (1)</summary><blockquote> `910-913`: _⚠️ Potential issue_ | _🟡 Minor_ **Show a user-facing error banner when hide/unhide fails.** Right now failures only log, so the user gets no feedback. Consider showing a global error banner in addition to logging. <details> <summary>🐛 Suggested fix</summary> ```diff +use crate::ui::components::MessageBanner; ``` ```diff if let Some(identity) = &self.selected_identity { let owner_id = identity.identity.id(); if let Err(e) = self.app_context.db.set_contact_hidden( &owner_id, &contact.identity_id, new_hidden, ) { + MessageBanner::set_global( + ui.ctx(), + &format!("Failed to update contact: {}", e), + MessageType::Error, + ); tracing::error!( "Failed to update contact: {}", e ); } else { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 910 - 913, The error handler that currently only calls tracing::error!("Failed to update contact: {}", e) should also surface a user-facing global error banner: keep the tracing::error log, then call or create a UI helper (e.g. show_error_banner or AppState::show_global_error) to display a concise message like "Failed to hide/unhide contact" plus the error details or a short code; do this in the same error branch where the variable e is available so the user sees feedback when the hide/unhide operation fails. ``` </details> </blockquote></details> <details> <summary>src/ui/tokens/unfreeze_tokens_screen.rs-115-132 (1)</summary><blockquote> `115-132`: _⚠️ Potential issue_ | _🟡 Minor_ **Update banner copy to match “unfreeze” semantics.** These messages still reference “burning,” which is misleading in the unfreeze flow. <details> <summary>Proposed wording fix</summary> ```diff - set_error_banner("Burning is not allowed on this token"); + set_error_banner("Unfreezing is not allowed on this token"); ``` ```diff - "You are not allowed to burn this token. Only the contract owner is.", + "You are not allowed to unfreeze this token. Only the contract owner is.", ``` ```diff - set_error_banner("You are not allowed to burn this token"); + set_error_banner("You are not allowed to unfreeze this token"); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/unfreeze_tokens_screen.rs` around lines 115 - 132, The error banners in the AuthorizedActionTakers match arms (variants NoOne, ContractOwner, Identity) use "burn" language but this flow is for unfreezing; update calls to set_error_banner to use unfreeze wording (e.g., "Unfreezing is not allowed on this token", "You are not allowed to unfreeze this token", and for ContractOwner: "You are not allowed to unfreeze this token. Only the contract owner is.") while keeping the same guard checks against identity_token_info and without changing control flow in those branches. ``` </details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
* refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com>
* build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
75c8e63 to
95a1809
Compare
Aligns elapsed display with the countdown timer which already adds 1 to avoid showing "0s" immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
95a1809 to
e0f0ce9
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…sage-display-applied
Resolves conflict in mint_tokens_screen.rs by combining both Warning handling from the base branch and refresh_banner clearing from HEAD. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge v1.0-dev (d45b3ee) into the unified message display branch, resolving 45 conflicted files across the codebase. Resolution strategy: - Keep the unified MessageBanner approach (design branch) for all screen-level message display, replacing v1.0-dev's inline colored labels and per-screen message fields - Keep simplified status enums without timestamp parameters, since MessageBanner handles elapsed-time display via BannerHandle - Merge v1.0-dev's MessageBanner enhancements: details/suggestion fields, collapsible details section, tracing-based logging, BannerState::new()/reset_to() constructors, distinct error icon - Merge v1.0-dev's batch refresh counting in IdentitiesScreen (pending_refresh_count, total_refresh_count, pluralized messages) - Merge v1.0-dev's ContactDetailsScreen database loading and Platform persistence via backend tasks - Merge v1.0-dev's improved UX text in ContactRequests (informative label instead of non-functional Cancel button) - Fix post-merge issues: remove references to deleted `message` field in ContactDetailsScreen, fix infinite recursion in display_message https://claude.ai/code/session_015EEVFee5cXpgaGASfZcAN3
…ssage-display-applied # Conflicts: # src/context/connection_status.rs # src/ui/network_chooser_screen.rs
…rmatting
The previous code used `format!("{err:?}")` which produced a String, then
`with_details()` applied `{:#?}` again — wrapping the output in quotes and
escaping inner characters. Passing `&err` directly lets Debug format once.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace "Burning" error messages that were copy-pasted from burn screen into freeze, destroy, and resume token screens with contextually correct messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace display_message() calls with MessageBanner::set_global() in screens where display_message() is now a side-effect-only handler and no longer displays messages directly. Affected screens: create_asset_lock_screen, wallets_screen (MineBlocks), address_table (export error), profile_screen (validation), contact_details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace double unwrap in transfer_screen refresh() with unwrap_or_else + MessageBanner error display, matching the pattern from withdraw_screen. SEC-002 tokens_screen skipped: the .expect() calls are only on compile-time embedded image data (include_bytes!) which is safe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nner Replace ~15 local ui_state.message assignments and custom render_message_banner() with MessageBanner::set_global() via the display_message() trait method. Remove the message field from UiState and the unused Color32 import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Upgrade BANNER_KEY_COUNTER from Relaxed to SeqCst ordering for future-proofing against multi-threaded usage - Log evicted banners at warn level in set_global() and replace_global() - Add comment explaining why show_global() always writes back Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All items tracked in the unified message display TODO have been addressed or moved to the review findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Document why with_details() accepts Debug (not Display): structured error context is more useful in diagnostic details panes - Document replace_global() fallback-to-add behavior as intentional - Add INTENTIONAL(SEC-003) marker for developer mode error details - Add INTENTIONAL(SEC-004) marker for BannerHandle Send+Sync safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add validate_signing_key() helper in tokens/mod.rs to eliminate duplicated signing key validation across 12 token screens - Move signing key validation BEFORE WaitingForResult state transition so users see immediate errors instead of loading spinner then error - Replace is_err()/unwrap() anti-pattern with idiomatic let-else blocks in freeze, mint, transfer, destroy_frozen_funds, unfreeze screens Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace &mut Option<String> error out-parameter with idiomatic Result<Option<Arc<RwLock<Wallet>>>, String>. Update 26+ callsites across identity, token, DashPay, and contract screens. Callsite patterns: unwrap_or_else with MessageBanner for user-visible errors, unwrap_or(None) where errors were previously silently ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove duplicate MessageBanner imports in create_asset_lock_screen and wallets_screen/mod. Fix needless_borrows_for_generic_args clippy lints in profile_screen, transfer_screen, and wallets_screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Apply 13 triaged review fixes plus 1 bug fix across 22 files: - Remove dead error state fields (backend_message, error_message, Error variant) - Replace .expect() panics with graceful fallback + MessageBanner in token screens - Fix missing MessageBanner::show_global() on contracts documents screen - Migrate DocumentActionScreen inline errors to MessageBanner - Replace unwrap_or(None) with error-reporting fallback in DashPay screens - Fix replace_global idempotency and use relaxed atomic ordering in banner - Extract shared set_error_banner helper for 8 token screens - Restore correct Some(0) wallet index semantics - Document BannerHandle lifecycle in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: validate token description length before sending to Platform Descriptions must be either empty or 3-100 characters long. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): validate token description by char count, not byte length String::len() counts UTF-8 bytes, causing multi-byte characters (CJK, emoji) to be miscounted against the 3–100 limit. Switch to chars().count() and update all UI labels to surface the minimum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Move BannerHandleExt and ResultBannerExt from banner_ext.rs into message_banner.rs where they belong. Merge take_and_clear() into OptionBannerExt trait (impl for Option<BannerHandle>) alongside or_show_error() for Option. Remove the separate banner_ext module and Clearable helper trait for simplicity. Apply review findings: DRY patterns (take_and_clear, or_show_error, load_identities_with_banner), fix .expect() panics in constructors, restore known_identities in refresh(), narrow pub field visibility, add ScreenLike doc comments, and update CLAUDE.md conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabiitai full review |
- Replace .expect() panics in TransferTokensScreen and ClaimTokensScreen constructors with graceful degradation via Option<QualifiedIdentity> and MessageBanner error display (PROJ-001 HIGH) - Fix CLAUDE.md referencing non-existent BannerHandleExt trait name, corrected to OptionBannerExt (PROJ-002 MEDIUM) - Update set_global to preserve message_type when same text appears with different severity (RUST-001 MEDIUM) - Standardize display_message to handle both Error and Warning across all 11 token screens (RUST-002 MEDIUM) - Replace 21 manual take().clear() patterns with take_and_clear() across 6 files (RUST-003 MEDIUM) - Remove unused OptionBannerExt::or_show_error method (RUST-004 MEDIUM) - Migrate update_token_config from old error_message pattern to set_error_banner closure (RUST-005 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use .as_ref() instead of .clone() in the ui() identity guard of TransferTokensScreen and ClaimTokensScreen. QualifiedIdentity (Identity + KeyStorage + BTreeMap + Vec) was being cloned 60x/sec; now only borrowed for display, with clones deferred to button-click paths that actually need ownership. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Mirrors ResultBannerExt::or_show_error but for Option<T>: if None, displays a global error banner with the given message. Enables concise patterns like: identities.first().cloned().or_show_error(ctx, "No identities loaded") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Standardize display_message to handle both Error and Warning across 13 non-token screens that were missed in iteration 1 (PROJ-001 MEDIUM) - Replace .expect() panic in AddKeyScreen::refresh() with graceful or_show_error() + unwrap_or_default() (PROJ-002 MEDIUM) - Rename OptionResultExt to OptionBannerShowExt to avoid confusion with ResultBannerExt (RUST-001 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Missed in the previous sweep — standardize display_message to handle both Error and Warning, matching all other screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Guard side effects with Error|Warning match, use take_and_clear(), and remove redundant MessageBanner::set_global() call in 4 screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssage-display-applied # Conflicts: # src/app.rs # src/ui/components/message_banner.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/context/connection_status.rs (1)
351-377:⚠️ Potential issue | 🟠 MajorHandle TaskResult::Error in chainlock polling to prevent stale connection state.
When
GetBestChainLocksfails (e.g., Config::load fails at line 169 ofsrc/backend_task/core/mod.rs), it sendsTaskResult::Error, whichhandle_task_resultignores entirely. This leavesrpc_onlineunchanged from its previous value, so a prior success can leave the state incorrectly showing "connected" even when polling has failed. The error appears only as a message banner, not reflected in the connection status indicator.Reset
rpc_onlineto false when handlingTaskResult::Errorfor chainlock-related tasks, or distinguish chainlock errors in the task result metadata to update state appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/connection_status.rs` around lines 351 - 377, handle_task_result currently only updates connection state on TaskResult::Success for chainlock messages, so when GetBestChainLocks returns TaskResult::Error the rpc_online flag can remain true; modify handle_task_result to also handle TaskResult::Error for chainlock-related tasks by calling self.set_rpc_online(false) (and self.refresh_state() if appropriate) when the failing task corresponds to GetBestChainLocks / CoreItem::ChainLock(s). Locate the match on TaskResult in handle_task_result and add an arm or branch that detects TaskResult::Error (or inspects task metadata indicating CoreItem::ChainLock/GetBestChainLocks) and clears rpc_online to prevent stale "connected" state. Ensure you reference and use set_rpc_online(false) and refresh_state() from the same context where update_from_chainlocks and the existing CoreItem::ChainLock handling live.src/ui/tools/platform_info_screen.rs (1)
271-293:⚠️ Potential issue | 🟠 MajorSingle-flight task tracking is inconsistent with current result handling.
Line 282 infers the completed task by scanning
active_tasks, and Line 292 clears all active tasks. If multiple requests are triggered, results can be mislabeled and loading can stop prematurely.💡 Proposed fix (enforce single in-flight request)
fn trigger_task( &mut self, task_type: PlatformInfoTaskRequestType, task_name: &str, ) -> AppAction { - if !self.active_tasks.contains(task_name) { + if self.active_tasks.is_empty() && !self.active_tasks.contains(task_name) { self.active_tasks.insert(task_name.to_string()); let task = BackendTask::PlatformInfo(task_type); return AppAction::BackendTask(task); } AppAction::None }for (task_id, button_text, task_type) in button_tasks { - let is_loading = self.active_tasks.contains(task_id); + let is_loading = !self.active_tasks.is_empty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/platform_info_screen.rs` around lines 271 - 293, The code infers which task completed by scanning self.active_tasks and then clears all active tasks, which can mislabel results if multiple requests are in-flight; to fix, enforce a single in-flight request or match completions to a tracked task id: introduce/use a single tracked identifier (e.g., replace or supplement self.active_tasks with a self.current_task_id or ensure only one entry is pushed) when dispatching tasks, on completion only set self.current_result and self.current_result_title if the completed task id equals self.current_task_id (or if active_tasks.len() == 1), and only clear that specific task (or reset self.current_task_id) instead of calling self.active_tasks.clear(); update the logic around the task_names loop, the place that inserts into self.active_tasks, and where current_result/current_result_title are assigned to perform this exact-match check.src/ui/wallets/send_screen.rs (1)
1116-1124:⚠️ Potential issue | 🟠 MajorError state lacks context and recovery guidance.
On Line 1116, the error branch only renders a bare “Dismiss” button. If the banner has already dismissed, users lose context for what failed.
💡 Suggested improvement
SendStatus::Error => { - // Error message is displayed by the global MessageBanner. - // Show a dismiss/retry option. + // Error details are in the global MessageBanner. + // Keep local context so recovery is obvious even after banner timeout. ui.add_space(10.0); - if ui.button("Dismiss").clicked() { + ui.label( + RichText::new("Send failed. See the banner above for details.") + .color(DashColors::ERROR), + ); + ui.add_space(6.0); + if ui.button("Back to form").clicked() { self.send_status = SendStatus::NotStarted; } ui.add_space(10.0); None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 1116 - 1124, The SendStatus::Error branch currently only shows a "Dismiss" button and loses error context if the global MessageBanner is gone; update the SendStatus::Error handling in send_screen.rs to surface the error and recovery actions by displaying a concise error summary (pulling from the same source MessageBanner uses or from a stored error field on the struct) and adding both a "Retry" button that re-attempts the send (calling the existing send method or state transition) and a "Dismiss" that clears self.send_status back to SendStatus::NotStarted; ensure the UI also offers a "Details" toggle or tooltip to reveal the full error message for advanced users so they don't lose context when the banner is closed.src/ui/dpns/dpns_contested_names_screen.rs (1)
1829-1845:⚠️ Potential issue | 🟠 MajorError path clears banners but leaves operation states stuck.
On Lines 1831-1834, error/warning handling does not reset
refreshing_statusor vote workflow status. This can block refresh retries and leave bulk voting in an in-progress state after failures.🩹 Suggested state-reset fix
fn display_message(&mut self, message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. if matches!(message_type, MessageType::Error | MessageType::Warning) { self.refresh_banner.take_and_clear(); self.vote_banner.take_and_clear(); + self.refreshing_status = RefreshingStatus::NotRefreshing; + if matches!( + self.bulk_vote_handling_status, + VoteHandlingStatus::CastingVotes | VoteHandlingStatus::SchedulingVotes + ) { + self.bulk_vote_handling_status = VoteHandlingStatus::Failed(message.to_string()); + } } if message.contains("Error casting scheduled vote") { self.scheduled_vote_cast_in_progress = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dpns/dpns_contested_names_screen.rs` around lines 1829 - 1845, In display_message, after clearing banners via refresh_banner.take_and_clear() and vote_banner.take_and_clear(), also reset the view and workflow state so retries and vote flows aren't left "in-progress": set self.refreshing_status back to the idle/ready variant your RefreshingStatus enum uses, set self.scheduled_vote_cast_in_progress = false (already done for the specific error) and also clear any global vote workflow flags (e.g., self.bulk_vote_in_progress = false) and iterate any in-progress entries in self.scheduled_votes and any vote_workflows collections to mark them as Failed (similar to how you set ScheduledVoteCastingStatus::Failed) so no operation remains stuck.src/ui/identities/add_existing_identity_screen.rs (1)
670-702:⚠️ Potential issue | 🟠 MajorClear the loading banner when local index validation fails.
On Line 673 a loading banner starts before parsing, but when parsing fails (Line 695), the
refresh_bannerhandle is never cleared. This leaves a stale “Loading identity...” banner despite no backend task being started.🛠️ Suggested fix
if ui.add(button).clicked() { - self.add_identity_status = AddIdentityStatus::WaitingForResult; self.success_message = None; - let handle = MessageBanner::set_global( - self.app_context.egui_ctx(), - "Loading identity...", - MessageType::Info, - ); - handle.with_elapsed(); - self.refresh_banner = Some(handle); // Parse identity index input if let Ok(identity_index) = self.identity_index_input.trim().parse::<u32>() { + self.add_identity_status = AddIdentityStatus::WaitingForResult; + let handle = MessageBanner::set_global( + self.app_context.egui_ctx(), + "Loading identity...", + MessageType::Info, + ); + handle.with_elapsed(); + self.refresh_banner = Some(handle); + let wallet_ref = self.selected_wallet.as_ref().unwrap().clone().into(); action = AppAction::BackendTask(BackendTask::IdentityTask( match self.wallet_search_mode { WalletIdentitySearchMode::SpecificIndex => { IdentityTask::SearchIdentityFromWallet(wallet_ref, identity_index) @@ } else { // Handle invalid index input + self.refresh_banner.take_and_clear(); self.add_identity_status = AddIdentityStatus::NotStarted; MessageBanner::set_global( self.app_context.egui_ctx(), "Invalid identity index", MessageType::Error, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_existing_identity_screen.rs` around lines 670 - 702, When parsing identity_index_input fails you must clear the loading banner so it doesn't remain visible; in the else branch after setting add_identity_status back to NotStarted, remove/close the existing MessageBanner handle stored in self.refresh_banner (e.g. take the Option, call the handle's close/clear method or call a MessageBanner::clear_global helper) and set self.refresh_banner to None before showing the "Invalid identity index" error via MessageBanner::set_global; update code touching self.refresh_banner and the else branch around identity_index_input parsing to do this.
♻️ Duplicate comments (1)
src/ui/wallets/create_asset_lock_screen.rs (1)
673-675:⚠️ Potential issue | 🟠 Major
display_messageno-op can leave the flow stuck after backend errors.When an error arrives during
WalletFundedScreenStep::WaitingForAssetLock, Line 673 currently does nothing, so the screen can remain in a perpetual waiting state.💡 Proposed fix (preserve global banner, restore local state)
fn display_message(&mut self, _message: &str, _message_type: MessageType) { - // Error/success display is handled by the global MessageBanner. + // Error/success display is handled by the global MessageBanner. + // Keep side-effects to prevent stale waiting states. + if matches!(_message_type, MessageType::Error | MessageType::Warning) { + self.is_creating = false; + if let Ok(mut step) = self.step.write() + && *step == WalletFundedScreenStep::WaitingForAssetLock + { + *step = WalletFundedScreenStep::FundsReceived; + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/create_asset_lock_screen.rs` around lines 673 - 675, display_message currently no-ops causing the UI to stay stuck during WalletFundedScreenStep::WaitingForAssetLock errors; modify display_message to forward the message to the global MessageBanner (preserve existing banner behavior) and also update the local screen state (e.g., clear the WaitingForAssetLock flag or set the screen step to an error/idle state) so the local flow can proceed or show retry UI; locate and update the display_message method in create_asset_lock_screen.rs and adjust state fields used by WalletFundedScreenStep::WaitingForAssetLock (or the local waiting flag) accordingly.
🟠 Major comments (20)
src/ui/dashpay/qr_scanner.rs-260-260 (1)
260-260:⚠️ Potential issue | 🟠 MajorDo not surface raw wallet errors directly to users.
This currently displays backend/internal error text as-is. Please sanitize to a stable user-facing message.
Proposed fix
- if let Err(e) = try_open_wallet_no_password(wallet) { - MessageBanner::set_global(ui.ctx(), &e, MessageType::Error); + if let Err(_e) = try_open_wallet_no_password(wallet) { + MessageBanner::set_global( + ui.ctx(), + "Unable to access the selected wallet. Please unlock it and try again.", + MessageType::Error, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_scanner.rs` at line 260, The code is passing the raw backend error variable `e` into `MessageBanner::set_global(ui.ctx(), &e, MessageType::Error)` which surfaces internal error text to users; change this to show a stable, sanitized user-facing message (e.g. "Unable to access wallet. Please try again or contact support.") while logging the original `e` to the application logs for diagnostics (use your existing logger/tracing facility). Update the call site in `qr_scanner.rs` (the `MessageBanner::set_global` invocation) to pass the sanitized string and add a separate log statement that records `e` at an appropriate level.src/ui/identities/keys/key_info_screen.rs-513-514 (1)
513-514:⚠️ Potential issue | 🟠 MajorDon’t surface raw internal errors directly in user banners.
These banners currently expose raw error strings from wallet/storage/validation paths. Prefer a user-safe message and log full details separately.
🔒 Suggested hardening
- MessageBanner::set_global(ui.ctx(), &e, MessageType::Error); + tracing::warn!("try_open_wallet_no_password failed: {}", e); + MessageBanner::set_global( + ui.ctx(), + "Unable to access wallet right now. Please try again.", + MessageType::Error, + );- MessageBanner::set_global( - self.app_context.egui_ctx(), - format!("Issue saving: {}", e), - MessageType::Error, - ); + tracing::warn!("update_local_qualified_identity failed: {}", e); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to save changes.", + MessageType::Error, + );Also applies to: 635-639, 651-655, 801-805
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 513 - 514, Several MessageBanner::set_global(...) calls are currently passing raw internal error strings to the UI; replace those raw errors with a concise, user-safe message like "An unexpected error occurred" (or context-specific friendly text) and send the full error details to your logging system instead (e.g., tracing::error! or log::error!) so internals aren't exposed. Locate each usage of MessageBanner::set_global in key_info_screen.rs (the calls noted around the current occurrences) and change the banner text to a sanitized message while calling the logger with the original error value for diagnostics. Ensure any error formatting is not displayed to users and reuse the same pattern for the other similar blocks in this file.src/ui/identities/keys/key_info_screen.rs-642-656 (1)
642-656:⚠️ Potential issue | 🟠 MajorPersist-first (or rollback) to avoid in-memory/persisted state divergence.
Both add/remove flows mutate
self.identity/self.private_key_databefore persistence. Ifupdate_local_qualified_identityfails, the UI state no longer reflects stored state.💡 Suggested fix (transactional update pattern)
- self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)); - self.identity.private_keys.insert_non_encrypted( + let mut updated_identity = self.identity.clone(); + updated_identity.private_keys.insert_non_encrypted( (self.key.purpose().into(), self.key.id()), (self.key.clone().into(), private_key_bytes), ); - if let Err(e) = self.app_context.update_local_qualified_identity(&self.identity) { + if let Err(e) = self.app_context.update_local_qualified_identity(&updated_identity) { MessageBanner::set_global( self.app_context.egui_ctx(), - format!("Issue saving: {}", e), + "Failed to save private key changes.", MessageType::Error, ); + return; } + self.identity = updated_identity; + self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None));- self.private_key_data = None; - self.identity.private_keys.private_keys.remove(&(self.key.purpose().into(), self.key.id())); - if let Err(e) = self.app_context.update_local_qualified_identity(&self.identity) { + let mut updated_identity = self.identity.clone(); + updated_identity + .private_keys + .private_keys + .remove(&(self.key.purpose().into(), self.key.id())); + if let Err(e) = self.app_context.update_local_qualified_identity(&updated_identity) { MessageBanner::set_global( ui.ctx(), - format!("Issue saving: {}", e), + "Failed to remove private key.", MessageType::Error, ); + return; } + self.identity = updated_identity; + self.private_key_data = None;Also applies to: 792-807
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 642 - 656, The code mutates in-memory state (self.private_key_data and self.identity via identity.private_keys.insert_non_encrypted) before calling update_local_qualified_identity, causing UI/state divergence if persistence fails; change to a transactional pattern: perform the persistence call update_local_qualified_identity first using the new key data (or construct the updated identity/keys to pass), and only on success update self.private_key_data and self.identity in memory (or if you must mutate first, catch errors and rollback by removing the inserted key from identity.private_keys and resetting self.private_key_data); refer to symbols identity.private_keys.insert_non_encrypted, self.private_key_data, and update_local_qualified_identity to locate and apply the fix (also apply same fix to the analogous block around lines 792-807).src/ui/components/message_banner.rs-309-331 (1)
309-331:⚠️ Potential issue | 🟠 MajorText-based dedup can alias independent operations to the same handle.
Line 316 returns the existing banner key for matching text. Then Line 775 clears by key, so one caller can clear another caller’s in-flight banner if both used identical text.
Also applies to: 772-776
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/message_banner.rs` around lines 309 - 331, The dedup logic in set_global (using get_banners / set_banners) returns an existing banner's key when text matches, allowing one caller to clear another caller's banner via BannerHandle.key; change this so text-only matches do not reuse the existing.key: when a banner with the same text is found, do not return its key—either update its message_type in-place but create a new Banner entry with a freshly generated key (or otherwise generate a new unique key for the caller) and push it to banners, then call set_banners(ctx, banners) and return a BannerHandle with the new key; reference the set_global function, existing.key, BannerHandle and get_banners/set_banners when locating where to implement the change.src/ui/tokens/freeze_tokens_screen.rs-333-338 (1)
333-338:⚠️ Potential issue | 🟠 MajorScope
display_messageside-effects to active freeze operations only.Lines 335-338 set
FreezeTokensStatus::Errorfor any warning/error. Unrelated global messages can incorrectly change this screen’s state.Suggested fix
fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. if matches!(message_type, MessageType::Error | MessageType::Warning) { - self.refresh_banner.take_and_clear(); - self.status = FreezeTokensStatus::Error; + if matches!(self.status, FreezeTokensStatus::WaitingForResult) { + self.refresh_banner.take_and_clear(); + self.status = FreezeTokensStatus::Error; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/freeze_tokens_screen.rs` around lines 333 - 338, display_message currently clears refresh_banner and sets status to FreezeTokensStatus::Error for any Warning/Error; restrict these side-effects to only when the screen has an active freeze operation. Update display_message (the method shown) to first check that this screen is handling an active freeze (e.g. verify a pending refresh via self.refresh_banner.is_some() or an existing active/freezing flag on the struct) and only then call self.refresh_banner.take_and_clear() and set self.status = FreezeTokensStatus::Error; otherwise ignore unrelated global messages. Ensure you reference the existing symbols display_message, self.refresh_banner, and FreezeTokensStatus::Error when making the change.src/ui/dashpay/qr_code_generator.rs-125-129 (1)
125-129:⚠️ Potential issue | 🟠 MajorSanitize backend error details before showing QR generation failures.
On Line 127, the raw error is shown directly to users. This can leak internal details and unstable diagnostics.
Suggested fix
Err(e) => { - MessageBanner::set_global( - self.app_context.egui_ctx(), - format!("Failed to generate QR code: {}", e), - MessageType::Error, - ); + tracing::error!("Failed to generate QR code: {e}"); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to generate QR code. Please try again.", + MessageType::Error, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/qr_code_generator.rs` around lines 125 - 129, The code currently passes the raw backend error into MessageBanner::set_global (call using self.app_context.egui_ctx() and format!("Failed to generate QR code: {}", e)), which can leak internal details; change it to show a generic, sanitized user-facing message like "Failed to generate QR code. Please try again or contact support." and move the detailed error into a developer log (e.g., use log::error! or processLogger.error) so MessageBanner only displays the generic text while the original error `e` is recorded in logs for debugging.src/ui/tools/transition_visualizer_screen.rs-394-399 (1)
394-399:⚠️ Potential issue | 🟠 MajorDo not reset broadcast state on unrelated warning/error messages.
Lines 394-397 currently clear
submit_bannerand reset toNotStartedfor any warning/error. This can interrupt active flow and re-enable submit prematurely.Suggested fix
- fn display_message(&mut self, _message: &str, message_type: MessageType) { + fn display_message(&mut self, message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. match message_type { MessageType::Success => { if matches!(self.broadcast_status, TransitionBroadcastStatus::Submitting) { self.submit_banner.take_and_clear(); self.broadcast_status = TransitionBroadcastStatus::Complete(Instant::now()); } } MessageType::Error | MessageType::Warning => { - self.submit_banner.take_and_clear(); - self.broadcast_status = TransitionBroadcastStatus::NotStarted; + if matches!(self.broadcast_status, TransitionBroadcastStatus::Submitting) { + self.submit_banner.take_and_clear(); + self.broadcast_status = + TransitionBroadcastStatus::Error(message.to_string(), Instant::now()); + } } MessageType::Info => {} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/transition_visualizer_screen.rs` around lines 394 - 399, The current match arm for MessageType::Error | MessageType::Warning unconditionally calls self.submit_banner.take_and_clear() and resets self.broadcast_status to TransitionBroadcastStatus::NotStarted, which disrupts active broadcasts; change this to only clear the banner and reset broadcast_status when the warning/error is actually related to the transition broadcast. Concretely, in the MessageType::Error | MessageType::Warning branch add a guard that verifies the submit_banner corresponds to a broadcast failure (e.g. check a new or existing metadata field on self.submit_banner, or inspect the banner message text via a helper like submit_banner.is_broadcast_error()) and only then call self.submit_banner.take_and_clear() and set self.broadcast_status = TransitionBroadcastStatus::NotStarted; otherwise leave the banner and broadcast_status untouched. Ensure you update or add the minimal helper on the submit_banner type if necessary to identify broadcast-related messages.src/ui/contracts_documents/document_action_screen.rs-512-516 (1)
512-516:⚠️ Potential issue | 🟠 MajorReset action state when document ID parsing fails.
Line 512 and Line 581 only show a banner; they don’t clear previously fetched state. That leaves stale
fetched_price/original_docpaths active and can enable unintended broadcasts after invalid input.Suggested fix
} else { MessageBanner::set_global( ui.ctx(), "Invalid Document ID format", MessageType::Error, ); + self.fetched_price = None; + self.broadcast_status = BroadcastStatus::NotBroadcasted; }} else { MessageBanner::set_global( ui.ctx(), "Invalid Document ID format", MessageType::Error, ); + self.original_doc = None; + self.field_inputs.clear(); + self.broadcast_status = BroadcastStatus::NotBroadcasted; }Also applies to: 581-585
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/document_action_screen.rs` around lines 512 - 516, When parsing the Document ID fails (where MessageBanner::set_global("Invalid Document ID format", MessageType::Error) is called) also reset the action state to avoid stale data: clear or set to None the fetched_price and original_doc fields and reset any broadcast/enabled flags or paths that depend on a valid document (e.g., disable/clear broadcast_path or set can_broadcast = false). Apply the same reset logic at the other parse-failure site where the banner is shown so both code paths clear fetched_price, original_doc and any broadcast-related state in the DocumentActionScreen struct before returning.src/ui/tokens/mod.rs-24-30 (1)
24-30:⚠️ Potential issue | 🟠 MajorDo not downgrade identity-load failures to an empty list.
Line 29 (
unwrap_or_default) masks load/corruption errors and allows token flows to proceed with silently missing identities. This should return/propagate the error so callers can bail explicitly.Suggested direction
-pub fn load_identities_with_banner(app_context: &AppContext) -> Vec<QualifiedIdentity> { +pub fn load_identities_with_banner( + app_context: &AppContext, +) -> Result<Vec<QualifiedIdentity>, String> { use crate::ui::components::ResultBannerExt; app_context .load_local_qualified_identities() .or_show_error(app_context.egui_ctx()) - .unwrap_or_default() }Then make callers early-return on
Err.Based on learnings: In
src/database/identities.rs, identity-loading paths intentionally abort on the first corrupted blob because skipping/masking corruption can lead to fund loss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/mod.rs` around lines 24 - 30, In load_identities_with_banner replace the current unwrap_or_default behavior so it returns a Result<Vec<QualifiedIdentity>, E> (propagating the error from app_context.load_local_qualified_identities().or_show_error(...)) instead of silently returning an empty list; update the function signature and callers to handle Err (early-return where identities are required) and preserve the use of or_show_error/app_context.egui_ctx() when converting or displaying the error from load_local_qualified_identities.src/app.rs-1030-1037 (1)
1030-1037:⚠️ Potential issue | 🟠 MajorDo not force all
BackendTaskSuccessResult::Messagepayloads into a success banner.Line 1035 always shows a success banner, but these message payloads are also interpreted as errors by some screens, causing contradictory UX (success flash followed by error). Route message severity explicitly instead of hardcoding
MessageType::Success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1030 - 1037, Currently BackendTaskSuccessResult::Message always calls MessageBanner::set_global with MessageType::Success, which forces every message into a success banner; instead, determine and pass the correct severity for the message (either by adding a severity field to BackendTaskSuccessResult::Message or by extracting severity from the existing unboxed_message) and call MessageBanner::set_global with that computed MessageType rather than MessageType::Success; update the BackendTaskSuccessResult::Message variant and the handling code (the match arm that calls MessageBanner::set_global and visible_screen_mut().display_task_result) so the banner reflects the message's explicit severity.src/ui/tools/masternode_list_diff_screen.rs-4149-4153 (1)
4149-4153:⚠️ Potential issue | 🟠 MajorRemove local error rendering side-effect from
display_messageto avoid duplicate banners.On Line 4152, setting
self.ui_state.errorstill triggers the inline error banner, so users can see both local and global errors for the same event. Keep only non-visual side effects here (e.g., clearing pending state).Suggested minimal fix
fn display_message(&mut self, message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. if matches!(message_type, MessageType::Error | MessageType::Warning) { self.task.pending = None; - self.ui_state.error = Some(message.to_string()); } }As per coding guidelines: "Use
MessageBannerfor user-facing messages and letisland_central_panel()render global banners centrally".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tools/masternode_list_diff_screen.rs` around lines 4149 - 4153, In display_message (the block matching message_type against MessageType::Error | MessageType::Warning) remove the visual side-effect that sets self.ui_state.error and only perform non-visual updates: keep self.task.pending = None but delete the assignment self.ui_state.error = Some(message.to_string()); let global banner rendering remain handled by AppState/MessageBanner and island_central_panel().src/ui/dashpay/add_contact_screen.rs-623-625 (1)
623-625:⚠️ Potential issue | 🟠 MajorDo not rely on fragile substring matching for backend error classification.
If backend wording changes, Line 626’s heuristic can miss errors and leave the screen stuck in
Sending. Add a fallback that always transitions out of sending state when a message arrives during an active send flow.Suggested guard to avoid sticky Sending state
BackendTaskSuccessResult::Message(message) => { // TODO(RUST-004): Replace string-based error matching with structured // error types through the task result system. This is fragile — if // upstream error wording changes, classification silently breaks. if message.contains("Error") || message.contains("Failed") || message.contains("does not have") { // Try to parse structured error, fallback to generic let error = if message.contains("ENCRYPTION key") { DashPayError::MissingEncryptionKey } else if message.contains("DECRYPTION key") { DashPayError::MissingDecryptionKey } else if message.contains("not found") && message.contains("username") { DashPayError::UsernameResolutionFailed { username: self.username_or_id.clone(), } } else if message.contains("Identity not found") { DashPayError::IdentityNotFound { identity_id: dash_sdk::platform::Identifier::from_string( &self.username_or_id, dash_sdk::dpp::platform_value::string_encoding::Encoding::Base58, ) .unwrap_or_else(|_| dash_sdk::platform::Identifier::random()), } } else if message.contains("Network") || message.contains("connection") { DashPayError::NetworkError { reason: message.clone(), } } else { DashPayError::Internal { message: message.clone(), } }; self.status = ContactRequestStatus::Error(error); + } else if matches!(self.status, ContactRequestStatus::Sending) { + self.status = ContactRequestStatus::Error(DashPayError::Internal { + message: message.clone(), + }); } // Ignore other messages - they're not for this screen }Also applies to: 626-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/add_contact_screen.rs` around lines 623 - 625, The current backend-error classification in add_contact_screen.rs relies on fragile substring matching of the "Sending" flow (see TODO(RUST-004)); modify the message-handling code (the branch that checks for backend error strings between lines ~626-659) to always clear the sending state as a fallback: whenever any backend message arrives while the UI is in the Sending state, set the UI to a non-sending state (e.g., mark sending=false or transition to Idle/Error/Success as appropriate) so the screen cannot become permanently stuck; keep the planned migration to structured task results (TODO(RUST-004>) but implement this unconditional fallback in the function that processes backend messages/ task results (the handler around the current substring check) and add a debug log for unexpected messages to aid future refactors.src/ui/contracts_documents/group_actions_screen.rs-262-268 (1)
262-268:⚠️ Potential issue | 🟠 MajorAvoid dropping fetched results on row-level action errors.
Line [262] and Line [275] set
fetch_group_actions_status = FetchGroupActionsStatus::Errorwhen preparing a single “Take Action” flow fails. That hides the already-fetched table and forces a refetch, even though fetch itself succeeded.Proposed fix
- self.fetch_group_actions_status = - FetchGroupActionsStatus::Error; MessageBanner::set_global( ui.ctx(), "No identity token balance found", MessageType::Error, ); @@ - self.fetch_group_actions_status = - FetchGroupActionsStatus::Error; MessageBanner::set_global( ui.ctx(), format!("Failed to get identity token info: {}", e), MessageType::Error, );Also applies to: 275-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/group_actions_screen.rs` around lines 262 - 268, The code currently sets self.fetch_group_actions_status = FetchGroupActionsStatus::Error in the row-level "Take Action" failure branches (around the blocks that call MessageBanner::set_global with "No identity token balance found"), which incorrectly hides the already-fetched table and forces a refetch; instead, remove the assignment to fetch_group_actions_status in those error paths and only emit the user-facing error via MessageBanner::set_global(ui.ctx(), ...). If you need to represent per-row action state, set or introduce a separate field (e.g., self.take_group_action_status or an ActionRowStatus enum) and update that instead of FetchGroupActionsStatus, leaving fetch_group_actions_status untouched when fetch succeeded. Ensure you update both error blocks mentioned (the one around lines 262 and the one around lines 275-281) and keep MessageBanner usage and ui.ctx() as-is.src/ui/dashpay/contact_info_editor.rs-288-290 (1)
288-290:⚠️ Potential issue | 🟠 MajorReset
savingon error/warning messages to avoid stuck spinner.After Line [97] sets
self.saving = true, backend failures typically arrive viadisplay_message. With Line [288] as a no-op, the screen can stay in “Saving...” indefinitely.Proposed fix
- pub fn display_message(&mut self, _message: &str, _message_type: MessageType) { + pub fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. + if matches!(message_type, MessageType::Error | MessageType::Warning) { + self.saving = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contact_info_editor.rs` around lines 288 - 290, The display_message method currently does nothing, causing self.saving to remain true after backend failures; update fn display_message(&mut self, _message: &str, message_type: MessageType) to clear the saving flag on non-success outcomes by setting self.saving = false when message_type indicates Error or Warning (leave Success/Info behaviors unchanged), and keep any existing note about global banner handling for AppState; reference the display_message method and the struct field saving to locate where to add this change.src/ui/dashpay/send_payment.rs-786-788 (1)
786-788:⚠️ Potential issue | 🟠 Major
PaymentHistorycan remain stuck in loading state on backend errors.Line [576] sets
loading = true, but Line [786] no longer clears it when failures arrive through the message path.Proposed fix
- pub fn display_message(&mut self, _message: &str, _message_type: MessageType) { + pub fn display_message(&mut self, _message: &str, _message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. + self.loading = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/send_payment.rs` around lines 786 - 788, The PaymentHistory display_message implementation currently does nothing, causing self.loading to stay true when backend errors are delivered via messages; update the PaymentHistory::display_message method in src/ui/dashpay/send_payment.rs to clear the loading flag (self.loading = false) and set an appropriate error state when a failure message arrives—inspect the MessageType enum (or the message text) to detect error/failure cases and ensure you update self.error or similar fields and reset any loading indicator so the UI unblocks.src/ui/contracts_documents/group_actions_screen.rs-460-466 (1)
460-466:⚠️ Potential issue | 🟠 MajorScope error side-effects to active fetch state.
Line [463] currently flips to
Errorfor any warning/error delivered to this screen. Unrelated global errors can clear a validComplete(...)result view.Proposed fix
- fn display_message(&mut self, _message: &str, message_type: MessageType) { + fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. - match message_type { - MessageType::Error | MessageType::Warning => { - self.fetch_banner.take_and_clear(); - self.fetch_group_actions_status = FetchGroupActionsStatus::Error; - } - MessageType::Success | MessageType::Info => {} + if matches!(self.fetch_group_actions_status, FetchGroupActionsStatus::WaitingForResult) + && matches!(message_type, MessageType::Error | MessageType::Warning) + { + self.fetch_banner.take_and_clear(); + self.fetch_group_actions_status = FetchGroupActionsStatus::Error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/group_actions_screen.rs` around lines 460 - 466, In display_message, avoid flipping fetch_group_actions_status to Error for unrelated global warnings/errors; instead, only perform the side-effects (self.fetch_banner.take_and_clear() and set fetch_group_actions_status = FetchGroupActionsStatus::Error) when the screen is actively fetching or in a non-final state—i.e., guard the MessageType::Error | MessageType::Warning arm with a check against the current self.fetch_group_actions_status (allow mutation only for FetchGroupActionsStatus variants representing in-progress/pending states, not Complete), so existing Complete results are not cleared by unrelated errors.src/ui/contracts_documents/contracts_documents_screen.rs-543-550 (1)
543-550:⚠️ Potential issue | 🟠 MajorDon’t key query failure state on message text.
Line [545] relies on
"Error fetching documents"substring matching. If backend wording changes, this screen can remainWaitingForResultwith stale elapsed banner and no state transition.Proposed fix
- fn display_message(&mut self, message: &str, message_type: MessageType) { + fn display_message(&mut self, _message: &str, message_type: MessageType) { // Banner display is handled globally by AppState; this is only for side-effects. - if message.contains("Error fetching documents") - && matches!(message_type, MessageType::Error | MessageType::Warning) - { + if matches!(self.document_query_status, DocumentQueryStatus::WaitingForResult) + && matches!(message_type, MessageType::Error | MessageType::Warning) + { self.query_banner.take_and_clear(); self.document_query_status = DocumentQueryStatus::Error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/contracts_documents/contracts_documents_screen.rs` around lines 543 - 550, display_message currently keys query failure logic on a string match of "Error fetching documents", which is brittle; update the handling to rely on a structured signal instead: add a dedicated variant like MessageType::QueryFailed (or MessageType::DocumentQueryError) to the MessageType enum and change display_message to check for that variant (matches!(message_type, MessageType::QueryFailed)) when deciding to call self.query_banner.take_and_clear() and set self.document_query_status = DocumentQueryStatus::Error; this ensures the screen transitions correctly without depending on backend wording.src/ui/dashpay/contacts_list.rs-1036-1040 (1)
1036-1040:⚠️ Potential issue | 🟠 MajorStop swallowing database write failures in contact sync paths.
These
let _ = ...calls suppress persistence errors and can leave cache/state silently inconsistent after a fetch or profile update.🛠️ Suggested fix pattern
- let _ = self - .app_context - .db - .clear_dashpay_contacts(&owner_id, &network_str); + if let Err(e) = self + .app_context + .db + .clear_dashpay_contacts(&owner_id, &network_str) + { + tracing::error!("Failed to clear cached contacts for {}: {}", owner_id, e); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to refresh contacts cache", + MessageType::Error, + ); + return; + } - let _ = self.app_context.db.save_dashpay_contact( + if let Err(e) = self.app_context.db.save_dashpay_contact( &owner_id, &contact_data.identity_id, &network_str, contact_data.username.as_deref(), contact_data.display_name.as_deref(), contact_data.avatar_url.as_deref(), None, "accepted", - ); + ) { + tracing::error!("Failed to save cached contact {}: {}", contact_data.identity_id, e); + }Also applies to: 1065-1074, 1078-1085, 1154-1163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 1036 - 1040, The code is currently swallowing persistence errors by using "let _ = self.app_context.db.clear_dashpay_contacts(&owner_id, &network_str);" which can leave cache/state inconsistent; change calls to clear_dashpay_contacts (and other DB write calls in the contact sync paths) to handle the Result: check for Err, log the error with context (include owner_id and network_str) via the existing logger, and propagate or return the error upstream (or at minimum mark sync as failed) instead of discarding it; apply the same pattern to the other occurrences around the contact sync (the calls at lines referenced in the review: the similar DB writes at 1065-1074, 1078-1085, 1154-1163).src/ui/dashpay/contacts_list.rs-1077-1085 (1)
1077-1085:⚠️ Potential issue | 🟠 MajorPersist private contact data even when nickname is empty.
Saving private info only when
nicknameis present dropsnote/is_hiddenupdates for contacts without a nickname.🛠️ Proposed fix
- if let Some(nickname) = &contact_data.nickname { - let _ = self.app_context.db.save_contact_private_info( - &owner_id, - &contact_data.identity_id, - nickname, - &contact_data.note.unwrap_or_default(), - contact_data.is_hidden, - ); - } + if contact_data.nickname.is_some() || contact_data.note.is_some() || contact_data.is_hidden { + let nickname = contact_data.nickname.as_deref().unwrap_or(""); + let note = contact_data.note.as_deref().unwrap_or(""); + if let Err(e) = self.app_context.db.save_contact_private_info( + &owner_id, + &contact_data.identity_id, + nickname, + note, + contact_data.is_hidden, + ) { + tracing::error!( + "Failed to save private contact info for {}: {}", + contact_data.identity_id, + e + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/dashpay/contacts_list.rs` around lines 1077 - 1085, The code only calls self.app_context.db.save_contact_private_info when contact_data.nickname is Some, which drops updates to note/is_hidden for contacts without nicknames; change it to always call save_contact_private_info using the contact_data.nickname defaulted to an empty string (e.g. via contact_data.nickname.as_deref().unwrap_or_default() or contact_data.nickname.unwrap_or_default()), passing owner_id, contact_data.identity_id, the computed nickname, &contact_data.note.unwrap_or_default(), and contact_data.is_hidden so note/is_hidden are persisted even when nickname is empty.src/ui/tokens/claim_tokens_screen.rs-85-97 (1)
85-97:⚠️ Potential issue | 🟠 MajorDo not fallback to an arbitrary identity when target identity is missing.
Line 96 falls back to the first local identity after an error. That can route a claim through the wrong identity/key set and produce unintended token operations.
🛠️ Suggested fix
- let identity = known_identities - .iter() - .find(|id| id.identity.id() == identity_token_basic_info.identity_id) - .cloned() - .or_else(|| { - MessageBanner::set_global( - app_context.egui_ctx(), - "Identity not found in local store", - MessageType::Error, - ); - // Fallback to first available identity for degraded state. - known_identities.first().cloned() - }); + let identity = known_identities + .iter() + .find(|id| id.identity.id() == identity_token_basic_info.identity_id) + .cloned(); + + if identity.is_none() { + MessageBanner::set_global( + app_context.egui_ctx(), + "Identity not found in local store", + MessageType::Error, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/claim_tokens_screen.rs` around lines 85 - 97, The code currently falls back to known_identities.first().cloned() when the identity lookup (using known_identities.iter().find(...) against identity_token_basic_info.identity_id) fails, which can route actions through the wrong key; remove that fallback and ensure the lookup returns None while still showing the error banner via MessageBanner::set_global(app_context.egui_ctx(), ...). Concretely: stop using or_else(...) that returns first().cloned(); instead perform the .find(...).cloned(), check if the result is None, call MessageBanner::set_global(...) to surface the error, and propagate/handle the None identity so no implicit fallback identity is used (update any downstream code to handle the Option accordingly).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
CLAUDE.mddocs/ai-design/2026-02-27-banner-review-fixes/pr-604-review-guide.mdsrc/app.rssrc/backend_task/core/mod.rssrc/context/connection_status.rssrc/ui/components/message_banner.rssrc/ui/components/mod.rssrc/ui/components/wallet_unlock.rssrc/ui/contracts_documents/add_contracts_screen.rssrc/ui/contracts_documents/contracts_documents_screen.rssrc/ui/contracts_documents/document_action_screen.rssrc/ui/contracts_documents/group_actions_screen.rssrc/ui/contracts_documents/register_contract_screen.rssrc/ui/contracts_documents/update_contract_screen.rssrc/ui/dashpay/add_contact_screen.rssrc/ui/dashpay/contact_details.rssrc/ui/dashpay/contact_info_editor.rssrc/ui/dashpay/contact_profile_viewer.rssrc/ui/dashpay/contact_requests.rssrc/ui/dashpay/contacts_list.rssrc/ui/dashpay/profile_screen.rssrc/ui/dashpay/profile_search.rssrc/ui/dashpay/qr_code_generator.rssrc/ui/dashpay/qr_scanner.rssrc/ui/dashpay/send_payment.rssrc/ui/dpns/dpns_contested_names_screen.rssrc/ui/identities/add_existing_identity_screen.rssrc/ui/identities/add_new_identity_screen/by_platform_address.rssrc/ui/identities/add_new_identity_screen/by_using_unused_asset_lock.rssrc/ui/identities/add_new_identity_screen/by_using_unused_balance.rssrc/ui/identities/add_new_identity_screen/by_wallet_qr_code.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/identities_screen.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/identities/mod.rssrc/ui/identities/register_dpns_name_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/identities/withdraw_screen.rssrc/ui/mod.rssrc/ui/network_chooser_screen.rssrc/ui/tokens/add_token_by_id_screen.rssrc/ui/tokens/burn_tokens_screen.rssrc/ui/tokens/claim_tokens_screen.rssrc/ui/tokens/destroy_frozen_funds_screen.rssrc/ui/tokens/direct_token_purchase_screen.rssrc/ui/tokens/freeze_tokens_screen.rssrc/ui/tokens/mint_tokens_screen.rssrc/ui/tokens/mod.rssrc/ui/tokens/pause_tokens_screen.rssrc/ui/tokens/resume_tokens_screen.rssrc/ui/tokens/set_token_price_screen.rssrc/ui/tokens/tokens_screen/contract_details.rssrc/ui/tokens/tokens_screen/keyword_search.rssrc/ui/tokens/tokens_screen/mod.rssrc/ui/tokens/tokens_screen/my_tokens.rssrc/ui/tokens/tokens_screen/structs.rssrc/ui/tokens/tokens_screen/token_creator.rssrc/ui/tokens/transfer_tokens_screen.rssrc/ui/tokens/unfreeze_tokens_screen.rssrc/ui/tokens/update_token_config.rssrc/ui/tokens/view_token_claims_screen.rssrc/ui/tools/address_balance_screen.rssrc/ui/tools/contract_visualizer_screen.rssrc/ui/tools/document_visualizer_screen.rssrc/ui/tools/grovestark_screen.rssrc/ui/tools/masternode_list_diff_screen.rssrc/ui/tools/platform_info_screen.rssrc/ui/tools/transition_visualizer_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/create_asset_lock_screen.rssrc/ui/wallets/send_screen.rssrc/ui/wallets/single_key_send_screen.rssrc/ui/wallets/wallets_screen/address_table.rssrc/ui/wallets/wallets_screen/dialogs.rssrc/ui/wallets/wallets_screen/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/ui/wallets/single_key_send_screen.rs
- src/ui/identities/add_new_identity_screen/by_using_unused_asset_lock.rs
- src/ui/tokens/tokens_screen/contract_details.rs
- src/ui/identities/add_new_identity_screen/by_platform_address.rs
- src/ui/wallets/wallets_screen/dialogs.rs
- src/ui/dashpay/contact_requests.rs
- src/ui/tools/document_visualizer_screen.rs
- src/ui/tokens/tokens_screen/mod.rs
| if let BackendTaskSuccessResult::Message(_) = result { | ||
| // Clear the form on success | ||
| self.qr_data_input.clear(); | ||
| self.parsed_qr_data = None; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=rust -C5 'SendContactRequestWithProof'Repository: dashpay/dash-evo-tool
Length of output: 2395
🏁 Script executed:
rg -nP --type=rust -C5 'BackendTaskSuccessResult::Message'Repository: dashpay/dash-evo-tool
Length of output: 17450
🏁 Script executed:
rg -nP --type=rust -C4 'fn display_task_result\(&mut self, result: BackendTaskSuccessResult\)'Repository: dashpay/dash-evo-tool
Length of output: 12738
🏁 Script executed:
sed -n '142,200p' src/backend_task/dashpay.rsRepository: dashpay/dash-evo-tool
Length of output: 2666
🏁 Script executed:
rg -n 'async fn send_contact_request_with_proof' --type=rust -A 50Repository: dashpay/dash-evo-tool
Length of output: 4728
🏁 Script executed:
rg -n 'display_task_result.*result' src/ui/dashpay/qr_scanner.rs -B 5 -A 10Repository: dashpay/dash-evo-tool
Length of output: 993
🏁 Script executed:
sed -n '175,400p' src/backend_task/dashpay/contact_requests.rs | tail -100Repository: dashpay/dash-evo-tool
Length of output: 3304
🏁 Script executed:
rg -n 'Ok\(BackendTaskSuccessResult' src/backend_task/dashpay/contact_requests.rsRepository: dashpay/dash-evo-tool
Length of output: 443
🏁 Script executed:
sed -n '490,510p' src/backend_task/dashpay/contact_requests.rsRepository: dashpay/dash-evo-tool
Length of output: 720
🏁 Script executed:
rg -n 'fn send_contact_request_with_proof' src/backend_task/dashpay/contact_requests.rs -A 330 | grep -E '^\d+-(.*Ok\(BackendTaskSuccessResult|^[0-9]+-\})'Repository: dashpay/dash-evo-tool
Length of output: 47
Form reset will never execute—wrong result type matched.
The SendContactRequestWithProof handler returns BackendTaskSuccessResult::DashPayContactRequestSent(...), but the condition at line 317 checks for BackendTaskSuccessResult::Message(_). These types never match, so the form clearing code (lines 319–320) is unreachable.
Match against DashPayContactRequestSent instead, or reconsider the result type returned by the handler. See add_contact_screen.rs (lines 610–620) for the correct pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/dashpay/qr_scanner.rs` around lines 317 - 320, The current match
checks BackendTaskSuccessResult::Message(_) which never matches the
SendContactRequestWithProof handler result; update the pattern in
src/ui/dashpay/qr_scanner.rs to match
BackendTaskSuccessResult::DashPayContactRequestSent(_) (or the exact payload
variant used by the SendContactRequestWithProof handler) so the form-clearing
lines self.qr_data_input.clear() and self.parsed_qr_data = None are reachable;
mirror the correct pattern used in add_contact_screen.rs
(DashPayContactRequestSent) to ensure the success branch executes.
| let identity = known_identities | ||
| .iter() | ||
| .find(|identity| identity.identity.id() == identity_token_balance.identity_id) | ||
| .expect("Identity not found") | ||
| .clone(); | ||
| .cloned() | ||
| .or_else(|| { | ||
| MessageBanner::set_global( | ||
| app_context.egui_ctx(), | ||
| "Identity not found in local store", | ||
| MessageType::Error, | ||
| ); | ||
| // Fallback to first available identity for degraded state. | ||
| known_identities.first().cloned() | ||
| }); |
There was a problem hiding this comment.
Do not substitute a different sender identity when lookup fails.
Line 85-Line 93 fall back to the first local identity if the expected identity is missing. That can submit a transfer with the wrong signer context for the selected token balance.
💡 Suggested fix
let identity = known_identities
.iter()
.find(|identity| identity.identity.id() == identity_token_balance.identity_id)
.cloned()
.or_else(|| {
MessageBanner::set_global(
app_context.egui_ctx(),
"Identity not found in local store",
MessageType::Error,
);
- // Fallback to first available identity for degraded state.
- known_identities.first().cloned()
+ None
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/tokens/transfer_tokens_screen.rs` around lines 81 - 93, The code
currently falls back to known_identities.first().cloned() when the lookup for
identity_token_balance.identity_id fails (in the block that builds identity from
known_identities), which may cause transfers to be signed by the wrong identity;
instead, stop substituting a different sender: remove the fallback to
known_identities.first().cloned(), keep the MessageBanner::set_global(...)
error, return or propagate None for identity (so the resulting variable is
Option::None) and ensure calling code (transfer UI/action) treats a missing
identity as an error/disabled state and does not attempt to sign or submit the
transfer; identify the affected symbols: known_identities,
identity_token_balance, MessageBanner::set_global and the local identity
variable, and make changes so failing lookup produces no implicit fallback
signer.
There was a problem hiding this comment.
Pull request overview
This PR implements a unified MessageBanner system for all screens, replacing dozens of per-screen inline status messages, elapsed-time labels, and error bubbles with a single centrally-rendered banner component.
Changes:
- Introduces
MessageBannercomponent with a global API (set_global,replace_global,clear_global_message),BannerHandlefor lifecycle management, and auto-dismiss/elapsed-timer support - Migrates ~50 screens from inline error/success labels and
WaitingForResult(timestamp)status variants to the unified banner system - Refactors
get_selected_walletto returnResult<Option<...>, String>instead of writing to an&mut Option<String>error parameter
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/ui/components/mod.rs |
Re-exports new banner extension traits |
src/ui/components/wallet_unlock.rs |
Removes set_error_message/error_message from trait; routes errors to global banner |
src/ui/mod.rs |
Changes display_task_result default to a no-op; adds doc comments to ScreenLike |
src/ui/identities/mod.rs |
Converts get_selected_wallet to Result-returning signature |
src/app.rs |
Routes task success/error to global banner; adds connection status banner management |
src/backend_task/core/mod.rs |
Removes early-return error for all-None chainlock result |
src/context/connection_status.rs |
Removes error branch from handle_task_result |
src/ui/tokens/mod.rs |
Adds shared helpers: load_identities_with_banner, set_error_banner, validate_signing_key |
src/ui/tokens/tokens_screen/structs.rs |
Simplifies can_transfer check |
| All screen files | Migrates inline messages/elapsed labels to global MessageBanner API |
CLAUDE.md |
Documents new banner architecture and BannerHandle lifecycle |
Comments suppressed due to low confidence (5)
src/ui/wallets/send_screen.rs:1
handle.with_elapsed()returns a value (likely&BannerHandleor a builder) but the return value is discarded. Ifwith_elapsed()is a builder method that must be stored to take effect, the elapsed timer will never start. The pattern used elsewhere in the codebase (e.g. intoken_creator.rs) callshandle.with_elapsed()and then storesself.operation_banner = Some(handle), which is what this function does — however, ifwith_elapsedreturns a new handle or mutates through a returned reference and the originalhandleis what gets stored, this is correct. But ifwith_elapsedis a consuming builder, the stored handle would be missing the elapsed configuration. Please verify the return type ofwith_elapsed()and confirm that storing the pre-callhandlecaptures the elapsed timer state.
src/ui/wallets/single_key_send_screen.rs:1- The
returnstatement was removed from this branch. Previously the function returned early after showing the fee confirmation dialog, preventing the message from being stored. Now execution continues to fall through to the end of the function, which no longer stores a message. While no message is stored (the storage was removed), thesendingstate reset logic above this block (lines 936–940) may now incorrectly execute for relay-fee errors —self.sending = falsewould be set even for relay-fee errors that should keep the sending state alive. The original comment said "Keep sending state true until user confirms or cancels", but removingreturnmeans thesendingstate check at lines 936–940 runs first and may clear it.
src/ui/tokens/tokens_screen/keyword_search.rs:1 - This manual
take-and-clearpattern is used in several other files viaself.operation_banner.take_and_clear()(using theOptionBannerExttrait). For consistency with the rest of the codebase, this should useself.operation_banner.take_and_clear()instead.
src/ui/wallets/single_key_send_screen.rs:1 self.fee_dialog.pending_request = Nonewas added here indisplay_task_resultbut the originaldisplay_messagecall (now replaced with a directset_global) was the mechanism that also cleared the fee dialog state in the original code. If a success result comes in while the fee dialog is open,pending_requestis cleared. However if the banner is set viaAppStateanddisplay_task_resultis called, there may be a double-clear or ordering issue depending on the call sequence. This is likely correct, but worth verifying thatpending_requestdoesn't need to be cleared indisplay_messageas well (for the error path).
src/ui/tools/transition_visualizer_screen.rs:1- Previously, when an error occurred the broadcast status was set to
TransitionBroadcastStatus::Error(message.to_string(), Instant::now()), which preserved the error message and timestamp for display. Now it resets toNotStarted, discarding the error info entirely. TheError(String, Instant)variant still exists in the enum (line 29 in the diff context) but is never set anymore. The fade-effect rendering for the error state (which was noted in the PR description as being intentionally preserved) would now never trigger. Please verify this is intentional or restore theErrorvariant assignment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
This is a follow-up PR implementing #601
MessageBannercomponent with global banner API (set_global,replace_global,clear_global_message) andBannerHandlefor lifecycle managementMessageType::Warningvariant and themed banner rendering (info/success/warning/error) with auto-dismiss, elapsed timer, and countdown supportisland_central_panel()—AppState::update()sets them automatically for backend task resultsAppAction::BackendTaskand optionally set aBannerHandle::with_elapsed()for long-running operationsChanges across the codebase
src/ui/components/message_banner.rs—MessageBanner,BannerHandle,BannerStatus, extension traits (OptionBannerExt,OptionBannerShowExt,ResultBannerExt)app.rsroutesTaskResultsuccess/error to global banners automatically; connection status banners trackOverallConnectionStatetransitions including newConnectingstateMessageBannerAPIWaitingForResult(timestamp)) toBannerHandle::with_elapsed()display_messagestandardized across all screens: side-effects only (clear progress banners, reset status), guarded byError | Warning, usingtake_and_clear().expect()/.unwrap()replaced withor_show_error()for graceful degradationQualifiedIdentityborrowed via.as_ref()instead of cloned every frame in token screensDashColorswith banner-specific color constantsMessageBannerReview iterations
display_messageimplementations for consistencySpecial cases handled
send_screen.rs: Kept full-screen spinner layout, removed only inline elapsed labeladd_existing_identity_screen.rs: Useshandle.set_message()for progress text updatesdpns_contested_names_screen.rs: Two separate banner handles (refresh + vote)tokens_screen/mod.rs: Sharedoperation_banneracross sub-screenstransition_visualizer_screen.rs: Only migratedSubmitting, keptInstant-based fade effectsmasternode_list_diff_screen.rs: Self-callsdisplay_message()for internal status updates; no redundantset_globalin trait implupdate_connection_banner(): Re-evaluates each frame inConnectingstate for degraded-peer timeout transitionsTest plan
cargo clippy --all-features --all-targets -- -D warningspassescargo +nightly fmt --allappliedcargo test --all-features --workspacepasses (42 tests)docs/ai-design/2026-02-17-unified-messages/manual-tests.md🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation